From a26f340566ded1383428822811e0b978b8887c86 Mon Sep 17 00:00:00 2001 From: Ralf Horstmann Date: Wed, 12 Feb 2020 17:19:01 +0100 Subject: [PATCH] Some more fixes for issues found by afl/asan (#500) * Fix uninitialized warning in lowranceusr * Fix stack buffer overlow in lowranceusr %15.10f may produce more characters than what fits into the buffer of 32 bytes. Use std::isnan to avoid the sprintf. ==2133227==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fff9bab5b20 at pc 0x7fc83fd95865 bp 0x7fff9bab57e0 sp 0x7fff9bab4f70 WRITE of size 38 at 0x7fff9bab5b20 thread T0 #0 0x7fc83fd95864 in vsprintf (/lib64/libasan.so.5+0x9e864) #1 0x7fc83fd95d1e in sprintf (/lib64/libasan.so.5+0x9ed1e) #2 0x767bbb in lowranceusr_parse_waypt /home/gpsbabel/gpsbabel.git/lowranceusr.cc:813 #3 0x77be54 in lowranceusr_parse_waypts /home/gpsbabel/gpsbabel.git/lowranceusr.cc:1116 #4 0x781151 in data_read /home/gpsbabel/gpsbabel.git/lowranceusr.cc:1658 #5 0xc7ac71 in run /home/gpsbabel/gpsbabel.git/main.cc:339 #6 0x4cdd62 in main /home/gpsbabel/gpsbabel.git/main.cc:707 #7 0x7fc83f29d1a2 in __libc_start_main (/lib64/libc.so.6+0x271a2) #8 0x4cef7d in _start (/home/gpsbabel/gpsbabel.git/gpsbabel+0x4cef7d) * Ensure string is terminated mps_readstr in mapsource When EOF or buffer limit is reached, the return buffer might not be zero-terminated. So add termination at the end of the buffer and read max sz - 1 bytes from file instead of sz bytes. ==2145172==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffd2ba34e00 at pc 0x7f005812dbbd bp 0x7ffd2ba34860 sp 0x7ffd2ba34008 READ of size 1028 at 0x7ffd2ba34e00 thread T0 #0 0x7f005812dbbc (/lib64/libasan.so.5+0x67bbc) #1 0x557c62 in QString::fromUtf8(char const*, int) /usr/include/qt5/QtCore/qstring.h:572 #2 0x557c62 in QString::operator=(char const*) /usr/include/qt5/QtCore/qstring.h:706 #3 0x557c62 in mps_waypoint_r /home/gpsbabel/gpsbabel.git/mapsource.cc:571 #4 0x55edd4 in mps_read /home/gpsbabel/gpsbabel.git/mapsource.cc:1675 #5 0xc7adb1 in run /home/gpsbabel/gpsbabel.git/main.cc:339 #6 0x4ce082 in main /home/gpsbabel/gpsbabel.git/main.cc:707 #7 0x7f005766c1a2 in __libc_start_main (/lib64/libc.so.6+0x271a2) #8 0x4cf29d in _start (/home/gpsbabel/gpsbabel.git/gpsbabel+0x4cf29d) Address 0x7ffd2ba34e00 is located in stack of thread T0 at offset 1344 in frame #0 0x556e8f in mps_waypoint_r /home/gpsbabel/gpsbabel.git/mapsource.cc:507 This frame has 6 object(s): [48, 56) 'wptdesc' (line 538) [80, 88) '' [112, 120) '' [144, 152) '' [176, 276) 'tbuf' (line 508) [320, 1344) 'wptname' (line 509) <== Memory access at offset 1344 overflows this variable * Fix off-by-one list access in pcx reader * Prevent stack based buffer overflow in gdb reader * Prevent global buffer overflow in garmin_fit The message_def array is 16 bytes long, make sure local_id stays within range, same as in other places in garmin_fit. Found by afl which managed to create a call to fit_parse_data_message with a header value of 0x1f: ==2927747==ERROR: AddressSanitizer: global-buffer-overflow on address 0x0000010a3bd8 at pc 0x000000a91bba bp 0x7ffd82584a90 sp 0x7ffd82584a80 READ of size 4 at 0x0000010a3bd8 thread T0 #0 0xa91bb9 in fit_parse_data /home/gpsbabel/gpsbabel.git/garmin_fit.cc:549 #1 0xa92ae3 in fit_parse_data_message /home/gpsbabel/gpsbabel.git/garmin_fit.cc:821 #2 0xa92ae3 in fit_parse_record /home/gpsbabel/gpsbabel.git/garmin_fit.cc:864 #3 0xa92ae3 in fit_read /home/gpsbabel/gpsbabel.git/garmin_fit.cc:884 #4 0xc7cdb1 in run /home/gpsbabel/gpsbabel.git/main.cc:339 #5 0x4ce142 in main /home/gpsbabel/gpsbabel.git/main.cc:707 #6 0x7f4f3b2651a2 in __libc_start_main (/lib64/libc.so.6+0x271a2) #7 0x4cf35d in _start (/home/gpsbabel/gpsbabel.git/gpsbabel+0x4cf35d) --- garmin_fit.cc | 2 +- gdb.cc | 2 ++ lowranceusr.cc | 10 +++++----- mapsource.cc | 3 ++- pcx.cc | 4 ++-- 5 files changed, 12 insertions(+), 9 deletions(-) diff --git a/garmin_fit.cc b/garmin_fit.cc index f43c750f7..9189f83b6 100644 --- a/garmin_fit.cc +++ b/garmin_fit.cc @@ -816,7 +816,7 @@ fit_parse_data(fit_message_def* def, int time_offset) static void fit_parse_data_message(uint8_t header) { - int local_id = header & 0x1f; + int local_id = header & 0x0f; fit_message_def* def = &fit_data.message_def[local_id]; fit_parse_data(def, 0); } diff --git a/gdb.cc b/gdb.cc index f1f239543..c7ca33874 100644 --- a/gdb.cc +++ b/gdb.cc @@ -401,6 +401,8 @@ read_file_header() } reclen = FREAD_i32; + is_fatal((reclen + 1 > int(sizeof(buf))), + MYNAME ": Invalid record length\n"); (void) FREAD(buf, reclen + 1); if (global_opts.verbose_status > 0) { const char* name = buf+2; diff --git a/lowranceusr.cc b/lowranceusr.cc index 740e9a6b0..dd9164bae 100644 --- a/lowranceusr.cc +++ b/lowranceusr.cc @@ -809,10 +809,7 @@ lowranceusr_parse_waypt(Waypoint* wpt_tmp, int object_num_present) */ if (rstream_version == 0) { float read_alt = gbfgetflt(file_in); - char buf[32]; - sprintf(buf, "%15.10f", read_alt); - // test for both cases to avoid compiler differences - if ((strcmp(buf, "-nan") == 0) || (strcmp(buf, "nan") == 0)) { + if (std::isnan(read_alt)) { wpt_tmp->altitude = unknown_alt; } else if (METERS_TO_FEET(wpt_tmp->altitude) <= -10000) { wpt_tmp->altitude = unknown_alt; @@ -1169,7 +1166,10 @@ static void lowranceusr4_parse_route() { int route_version; - int UUID1, UUID2, UUID3, UUID4; + int UUID1 = 0; + int UUID2 = 0; + int UUID3 = 0; + int UUID4 = 0; lowranceusr4_fsdata* fsdata = lowranceusr4_alloc_fsdata(); fs_chain_add(&(rte_head->fs), (format_specific_data*) fsdata); diff --git a/mapsource.cc b/mapsource.cc index 54d4a7b41..7ba2c283f 100644 --- a/mapsource.cc +++ b/mapsource.cc @@ -312,7 +312,8 @@ static void mps_readstr(gbfile* mps_file, char* buf, size_t sz) { int c; - while (sz-- && (c = gbfgetc(mps_file)) != EOF) { + buf[sz-1] = 0; + while (--sz && (c = gbfgetc(mps_file)) != EOF) { *buf++ = c; if (c == 0) { return; diff --git a/pcx.cc b/pcx.cc index 8649b5e58..e982d18a1 100644 --- a/pcx.cc +++ b/pcx.cc @@ -117,7 +117,7 @@ static void data_read() { case 'W': { QStringList tokens = line.split(QRegExp("\\s+"), QString::KeepEmptyParts); - if (tokens.size() < 5) { + if (tokens.size() < 6) { fatal(MYNAME ": Unable to parse waypoint, not all required columns " "contained\n"); @@ -217,7 +217,7 @@ static void data_read() { case 'T': { QStringList tokens = line.split(QRegExp("\\s+"), QString::KeepEmptyParts); - if (tokens.size() < 5) { + if (tokens.size() < 6) { fatal(MYNAME ": Unable to parse trackpoint, not all required columns " "contained\n"); -- 2.30.2